Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Base clojure template #19

Merged
merged 15 commits into from
Jun 9, 2024
Merged

Base clojure template #19

merged 15 commits into from
Jun 9, 2024

Conversation

marcitqualab
Copy link
Contributor

Hello, this is the first version of the baseline clojure template.

@behrica
Copy link
Member

behrica commented May 27, 2024

Looks good.
There is of course no precise answer what "minimal" means.
Just 2 comments:
In this line you hardcode a version of clj-new..

devcontainer-templates/src/basecloj/.devcontainer/Dockerfile

Line 26 in 451edf3

clojure -Ttools install io.github.seancorfield/clj-new '{:git/tag "v1.2.404" :git/sha "d4a6508"}' :as clj-new && \

This means "conceptually", that each new version of clj-new requires an update of the template.
(If you do them or not, is then an other question)

Regarding sshd, I would prefer to remove it, as this is "security" related.
I would prefer not to have a default devcontainer config which adds an accessible ssh server.
So please remove this:

devcontainer-templates/src/basecloj/.devcontainer/devcontainer.json

Line 19 in 451edf3

"ghcr.io/devcontainers/features/sshd:1": {}

(does this related to

devcontainer-templates/src/basecloj/.devcontainer/devcontainer.json

Line 12 in 451edf3

"--security-opt",
?
, if yes please remove as well)

@behrica behrica mentioned this pull request May 27, 2024
Open
@marcitqualab
Copy link
Contributor Author

marcitqualab commented May 27, 2024 •
edited
Loading

Hello @behrica ,

Yes the clj-new version for now is hardcoded. The official readme file recommends this way to install it. I have created a task on my side to research how to auto-update this version over time using the github actions.

I have removed the ssh and security options.

Many thanks,

@behrica behrica self-assigned this May 27, 2024
@behrica
Copy link
Member

behrica commented May 27, 2024

Changes looks good.
Do you still want to make the image tag configurable ?
as I commented on this file : src/basecloj/.devcontainer/Dockerfile

@marcitqualab
Copy link
Contributor Author

Hello @behrica ,

I do not see any specific comment in the file : src/basecloj/.devcontainer/Dockerfile.
If you are referring to the clj-new version tag, I think that for now we can start with the current latest version as it is and I will research and test how to implement an auto update or use latest tag if possible.
I can create a new PR later on when I have an update on how to remove the hard coded clj-new version tag.
Would that work for you?

@behrica
Copy link
Member

behrica commented May 28, 2024

No ,I talk about the "tag" of the base image
in this line in teh Dockerfile:

FROM clojure:temurin-21-tools-deps-jammy

you have hardcoded the tag of the image, namely:
temurin-21-tools-deps-jammy

But there are lots of others:
https://hub.docker.com/_/clojure/tags

And I think we should allow the user to "choose" when using the template.
This can be done via "template variables".
If we have those, VSCode renders the options and lets the user pick one,
the moment he is using the template.

The question is then:

  • which options we make "choosable"
  • whats the default ?

See here
https://github.com/scicloj/devcontainer-templates/blob/main/src/scicloj/devcontainer-template.json
what I think is the best solution for the "which options", (only LTS and JDK as supported by Clojure).
In any case a user can override it after applying the template.
But In VSCode he will only "see" the options we explicitly list.

@marcitqualab
Copy link
Contributor Author

Ah, yes, got it.
I will create some options then.
Thanks.

behrica
behrica approved these changes Jun 9, 2024
View reviewed changes
src/basecloj/.devcontainer/Dockerfile Outdated
@@ -0,0 +1,31 @@
FROM clojure:temurin-21-tools-deps-jammy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "tag" of the image should be configurable.
Please see here:

devcontainer-templates/src/scicloj/devcontainer-template.json

Line 8 in 451edf3

"options": {

and

devcontainer-templates/src/scicloj/.devcontainer/Dockerfile

Line 1 in c9ef0b7

FROM clojure:${templateOption:imageTag}

src/basecloj/.devcontainer/devcontainer.json Outdated
"USERNAME": "${localEnv:USER}"
}
},
"runArgs": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plase remove this, if it is here due to sshd which should be removed

src/basecloj/.devcontainer/Dockerfile

# [Optional] Set the default user. Omit if you want to keep the default as root.
USER $USERNAME
SHELL ["/bin/bash", "-ec"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried as well to make "bash" the defaut shell, but did not succeed neither.When I open a "terminal" in VSCode, I get "sh" first and not "bash".

If you are aware of a way to get a "bash" by default, let me know.

@behrica behrica merged commit 0676a77 into scicloj:main Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behrica behrica behrica approved these changes

Assignees

@behrica behrica

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcitqualab @behrica @marcandreuf